I know this is in draft, but let me tell you already that using "endsWith" is not the way to go here. hrefs are structured strings, composed by a namespace and an id. They have this format:

• #id; or
• namespace#id; where namespace is an URL.

We have a special parsing method to treat those and separate namespace and id. See parseXmlHref.

This is not a mutation error. Please adjust the logs.

Not in a mutation. Please adjust the logs.

Not in a mutation. Please adjust the logs.

Not inside a mutation.

I think we should throw here, as at this point, this operation should never fail. If edgeIndex doesn't exist, you create a new edge, thus querying for the edgeId again should always give you an edge. If that doesn't happen, something exceptional happened, so we throw. Also not in a mutation.

I think it's worth to add a comment here explaining why you're getting dmnEdgeIndex when you already have edgeIndex in scope.

Probably worth changing the name of this parameter to dmnEdgeIndex, since it represents a DMNEdge object.

This way, it is clear that false is the default for when extraArg is undefined.

export function _checkIsValidConnection(
  sourceNode: { type?: string; data: DmnDiagramNodeData } | undefined,
  targetNode: { type?: string; data: DmnDiagramNodeData } | undefined,
  edgeType: string | null | undefined,
  extraArg?: { allowExternalTarget: boolean; }
) {
  if (!sourceNode?.type || !targetNode?.type || !edgeType) {
    return false;
  }

  // External nodes cannot be targeted by default. However there are exceptions,
  // for example adding a waypoint on an edge between two external nodes.
  const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix != undefined;
  const allowExternalTarget = extraArg?.allowExternalTarget ?? false;
  if (isTargetExternalNode && !allowExternalTarget) {
    return false;
  }
}

Why not

?

External nodes are currently using an HREF (namespace#id) as ID, I think we should keep the same strategy for edges, instead of using a QName (prefix:id).

You can remove the definitions parameter you added too, if that ^ makes sense.

@tiagobento thank you for this topic. actually, nodes/shapes, currently uses prefix:id , not namespace#id, that is why implemented that way for edges.

Applying your suggestion produces xml like attached:

 <dmndi:DMNDI>
    <dmndi:DMNDiagram id="_B90C2A89-A2E3-4610-AC66-A6DC0F9C1CEF" name="Default DRD" useAlternativeInputDataShape="false">
      <di:extension>
        <kie:ComponentsWidthsExtension>
        </kie:ComponentsWidthsExtension>
      </di:extension>
      <dmndi:DMNEdge id="_D763F5B1-FAF0-4790-A905-A5B1D63F0D2C" dmnElementRef="included2:_90FB4683-E1A0-4149-A1B8-444DEE06E749" sourceElement="_A9428417-FA8E-47B7-9F4A-10921FEA34BA" targetElement="_CAFE156E-4FB5-4285-9079-E3165FB92DDE">
        <di:waypoint x="1300" y="260" />
        <di:waypoint x="900" y="240" />
        <di:waypoint x="600" y="400" />
      </dmndi:DMNEdge>
      <dmndi:DMNShape id="_CD133A74-86D5-4DA7-81BF-F122A69A7927" dmnElementRef="included0:_2081EEE3-7B1B-48B3-B3B9-944F0A013CF1">
        <dc:Bounds x="960" y="160" width="160" height="80" />
      </dmndi:DMNShape>
      <dmndi:DMNShape id="_AA2DC9CB-C337-4E8F-8258-ECA14E0CDB10" dmnElementRef="included0:_92040BB1-AB7A-46D1-A4DE-A4FD1448ADF5">
        <dc:Bounds x="700" y="220" width="160" height="80" />
      </dmndi:DMNShape>
      <dmndi:DMNEdge id="_68653841-7F18-46EB-B3F0-37082CC0A3E8" dmnElementRef="https://kie.org/dmn/_AE5E9361-3580-4587-AE13-53A36057853B#_0A18CF15-F3E5-4B87-82A1-2851A246FDF4" sourceElement="_AA2DC9CB-C337-4E8F-8258-ECA14E0CDB10" targetElement="_CD133A74-86D5-4DA7-81BF-F122A69A7927">
        <di:waypoint x="780" y="260" />
        <di:waypoint x="940" y="260" />
        <di:waypoint x="860" y="200" />
        <di:waypoint x="1040" y="200" />
      </dmndi:DMNEdge>
    </dmndi:DMNDiagram>
  </dmndi:DMNDI>

from this discussion my understanding is, I should update the code to use namespace#id also for nodes/shapes? Is that correct?

Ok, so let's take a step back. This id here is only the ID of the RF.Edge object we're telling ReactFlow exists. How it makes its way to the DMN XML is another story. I believe that ReactFlow objects (nodes and edges) should have their id in the HREF (I.e., namespace#id) format. That's one thing.

Now, you're correct to say that it is wrong to have an HREF in the dmnElementRef attribute, as, per the XSD, it's a QName (I.e., prefix:id). Now, let's try and see how the id of the RF.Edge is getting into the dmnElementRef....

The only place that adds DMNEdges to the XML is the addEdge mutation, so it must be an invocation to it. You added a new invocation to it on this PR... and there's this line:

A. requirementEdgeTargetingExternalNodeId: targetsExternalNode ? edgeId : undefined

See how the edgeId is now inside the addEdge mutation? Now let's look how this new requirementEdgeTargetingExternalNodeId parameter is used inside the addEdge mutation:

B. let existingEdgeId: string | undefined = requirementEdgeTargetingExternalNodeId;; and
C. "@_dmnElementRef": existingEdgeId ?? newEdgeId,

Hmm... something is not right :)

Now, all this is really confusing, because there are multiple ""types"" of id, and there are multiple formats for ids. We have:

1. The id of the ReactFlow object (I.e., RF.Edge), which I'm saying should be in HREF format.
2. The id attribute (@_id) of the Information Requirement (which is a plain UUID).
3. The id attribute (@_id) of the DMNEdge (which is a plain UUID).
4. The id we need to write to dmnElementRef attribute (@_dmnElementRef), which is in QName format.

They're all "ids" in some way, so it's really hard to name things properly.

Ok, let's go back.

I'd say:

• A. should be:
  dmnElementRefOfDmnEdge: targetsExternalNode ? informationRequirementQNameRelativeToThisDmn : undefined (this renaming requirementEdgeTargetingExternalNodeId)
• B. should be:
  let existingRequirementOrAssociationId: string | undefined; (thus renaming existingEdgeId)
• C. should be:
  "@_dmnElementRef": dmnElementRefOfDmnEdge ?? existingRequirementOrAssociationId ?? newEdgeId

And I believe we should rename newEdgeId to newRequirementOrAssociationId.

Let me know if you have questions :)

Btw, we have xmlHrefToQName to help you on building informationRequirementQNameRelativeToThisDmn from edgeId (I.e., Information Requirement's HREF relative to this DMN`).